Pull Request Test Coverage Report for Build 3060832130

• 153 of 242 (63.22%) changed or added relevant lines in 9 files are covered.
• 9 unchanged lines in 5 files lost coverage.
• Overall coverage decreased (-0.3%) to 74.671%

Totals
Change from base Build 2968537322:	-0.3%
Covered Lines:	5050
Relevant Lines:	6763

💛 - Coveralls

Thanks @dcbw for this. I've been wanting to do it since #259

@jcaamano so we now have size checking in a couple of places:

1. ovs-to-native, native-to-ovs
2. SetField()

Unmarshal of the OvsSet can't know the column maxSize, but whenever we unmarshall we're going to use that set to update our table cache, and that uses ovs-to-native (which is size-checked) and SetField (which is also size checked). So client/server will receive a Set as a row update, and then attempt to Merge/Update/etc that too-large row update into its existing row-cache, and then that will fail.

eg, fail when applying to the cache, not fail-when-unmarshalling.

I have the feeling that we are not implementing the size check in the correct place (and I probably misguided you here, sorry for that) and is a bit too spread over the place so as to ensure we are not making any mistake, or at least for me is difficult to track this down right now.

So what I can see is that OvsToNative, NativeToOvs and OvsSet can be used for things that should not be schema sized constrained in the same way as column values, like mutation or condition values. So I would just ignore these for size checks.

I hope that this size check can be contained in a single place for simplicity and for this right now I am looking at the Mapper methods:

• GetRowData or whereabouts for the OVS to Native Path, which happens at the server and at the client processing server updates. While it is initially reasonable to think that doing size checks in client side of this path is not problematic, we might consider not doing so if a client and a server can run on different schema versions and an increase in size could be a backward compatible schema change. Perhaps we can have a flag to avoid doing it on the client
• NewRow or SetField or whereabouts for the Native to OVS path which happens at the client and the server when sending updates to the client. If using SetField, we should take care that it is not used to incrementally update the value of a column as it is done in ApplyModifications as it can temporarily violate the size constrain:
  

  libovsdb/cache/cache.go

  Lines 1314 to 1344 in 999ca19

  	for i := 0; i < nv.Len(); i++ {
  	// search for match in base values
  	baseValue, err := info.FieldByColumn(k)
  	if err != nil {
  	return modified, err
  	}
  	bv := reflect.ValueOf(baseValue)
  	var found bool
  	newVal := nv.Index(i).Interface()
  	for j := 0; j < bv.Len(); j++ {
  	if bv.Index(j).Interface() == newVal {
  	// found a match, delete from slice
  	found = true
  	newValue := reflect.AppendSlice(bv.Slice(0, j), bv.Slice(j+1, bv.Len()))
  	err = info.SetField(k, newValue.Interface())
  	if err != nil {
  	return modified, err
  	}
  	modified = true
  	break
  	}
  	}
  	if !found {
  	newValue := reflect.Append(bv, nv.Index(i))
  	err = info.SetField(k, newValue.Interface())
  	if err != nil {
  	return modified, err
  	}
  	modified = true
  	}
  	}

  
  Also we probably need to return proper ovsdb errors in transact rather than panicking.

Would it be reasonable to divide the PR in two:

• change arrays to slices
• implement size checks

I can take the second and you can get unblocked on the first. The cost is a server that misbehaves in a different way, but I don't think is working correctly as it is now.

• Also we probably need to return proper ovsdb errors in transact rather than panicking.

@jcaamano we can do that, it's not hard. But note that since the server doesn't actually implement transactions, if you have a problem halfway through an update, your cache is now inconsistent and we can't roll it back.

• Also we probably need to return proper ovsdb errors in transact rather than panicking.

@jcaamano we can do that, it's not hard. But note that since the server doesn't actually implement transactions, if you have a problem halfway through an update, your cache is now inconsistent and we can't roll it back.

Hmm in my eyes it does, but not sure if we are talking about the same thing or if I am missing something.
In the server side, we have transact.go that basically generates a set of updates (which we can validate). These updates (if validation is ok) are then committed to the server database (a operation that can't fail) and sent to the client.

What do you think about

Would it be reasonable to divide the PR in two:

* change arrays to slices
* implement size checks
I can take the second and you can get unblocked on the first. The cost is a server that misbehaves in a different way, but I don't think is working correctly as it is now.

Implementing size checks can be assumed under this effort: #338 with actually something we are going to work in sooner rather than later